Add per-user submission rate limit (1 per hour)#436
Conversation
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
This PR adds a global per-user submission rate limit (1 submission per hour) enforced at the API layer, backed by a new database query to detect recent submissions.
Changes:
- Added
LeaderboardDB.check_user_rate_limit()to fetch a user’s most recent submission within a configurable time window. - Enforced the per-user limit in
to_submit_info()by returning HTTP 429 with guidance about using the NVIDIA runner. - Added unit tests covering the DB rate-limit query and API behavior under rate-limited vs allowed conditions.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/test_leaderboard_db.py | Adds DB-level tests for the new per-user rate limit query. |
| tests/test_admin_api.py | Adds API tests asserting 429 on repeat submissions and non-429 on first submission. |
| src/libkernelbot/leaderboard_db.py | Introduces check_user_rate_limit() query used by the API to enforce the window. |
| src/kernelbot/api/api_utils.py | Enforces the per-user rate limit and returns 429 with a helpful client-facing message. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/kernelbot/api/api_utils.py
Outdated
| except Exception as e: | ||
| raise HTTPException( | ||
| status_code=500, | ||
| detail=f"Internal server error while checking rate limit: {e}", | ||
| ) from e |
There was a problem hiding this comment.
The 500-path includes the raw exception message in the HTTP response (...: {e}). This can leak internal details to clients. Prefer logging the exception server-side and returning a generic error message (optionally with a request ID) instead.
| """ | ||
| SELECT submission_time | ||
| FROM leaderboard.submission | ||
| WHERE user_id = %s | ||
| AND submission_time > NOW() - INTERVAL '%s hours' | ||
| ORDER BY submission_time DESC | ||
| LIMIT 1 | ||
| """, |
There was a problem hiding this comment.
The new rate-limit query filters on (user_id, submission_time) but there’s no supporting index in the schema migrations (currently only an index on submission(leaderboard_id)). At scale this will devolve into scanning many rows per request. Add a DB migration to create an index such as (user_id, submission_time DESC) (or (user_id, submission_time)) to keep the check fast.
src/kernelbot/api/api_utils.py
Outdated
| with db_context as db: | ||
| last_submission_time = db.check_user_rate_limit(user_id) | ||
| if last_submission_time: | ||
| raise HTTPException( | ||
| status_code=429, | ||
| detail=( | ||
| f"Rate limit exceeded. You can submit once per hour. " | ||
| f"Last submission: {last_submission_time.isoformat()}. " | ||
| f"Consider using the NVIDIA runner instead of Modal for faster iteration." | ||
| ), | ||
| ) |
There was a problem hiding this comment.
to_submit_info opens db_context twice (once for rate limiting, again for leaderboard/GPU validation). Because LeaderboardDB.__exit__ disconnects when refcount hits 0, this will connect/disconnect twice per request. Consider using a single with db_context as db: block for both checks to avoid extra connection overhead and reduce failure points.
| with db_context as db: | |
| last_submission_time = db.check_user_rate_limit(user_id) | |
| if last_submission_time: | |
| raise HTTPException( | |
| status_code=429, | |
| detail=( | |
| f"Rate limit exceeded. You can submit once per hour. " | |
| f"Last submission: {last_submission_time.isoformat()}. " | |
| f"Consider using the NVIDIA runner instead of Modal for faster iteration." | |
| ), | |
| ) | |
| last_submission_time = db_context.check_user_rate_limit(user_id) | |
| if last_submission_time: | |
| raise HTTPException( | |
| status_code=429, | |
| detail=( | |
| f"Rate limit exceeded. You can submit once per hour. " | |
| f"Last submission: {last_submission_time.isoformat()}. " | |
| f"Consider using the NVIDIA runner instead of Modal for faster iteration." | |
| ), | |
| ) |
msaroufim
force-pushed
the
per-user-submission-rate-limit
branch
from
d3c86d1 to
8017fef
Compare
Enforce a per-user rate limit of 1 submission per hour, scoped to Modal B200 GPU submissions on leaderboard ID 730 only. The 429 response suggests using the NVIDIA runner instead of Modal for faster iteration.
msaroufim
force-pushed
the
per-user-submission-rate-limit
branch
from
8017fef to
2c3fb71
Compare
2 participants
Enforce a global per-user rate limit of 1 submission per hour across all leaderboards and modes. The 429 response suggests using the NVIDIA runner instead of Modal for faster iteration.